fix(cli): accept reordered agent report arguments - #2928
Conversation
📝 WalkthroughWalkthroughThe pane reporting commands now accept pane IDs after options, support ChangesPane report argument parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The commands now accept pane IDs before or after options and support equals-form options, but the changelog still describes only one ordering, which may mislead users and integrations. The PR is mergeable with an update to the changelog. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ec399cc-948c-4018-a316-1219932c29d3
📒 Files selected for processing (3)
docs/next/CHANGELOG.mdsrc/cli/pane.rstests/cli/panes.rs
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.
|
@akbash-bot the implementation review is clean. please rebase onto current master, resolve the changelog conflict, and rerun checks and both review bots. |
9d30965 to
03c8baa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0fa6371-7911-45e4-9ba4-dd8dbefec48f
📒 Files selected for processing (1)
docs/next/CHANGELOG.md
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| - Tab bar status commands now remove ESC-prefixed terminal control sequences instead of displaying their sequence bodies as text. (#3001) | ||
| - Unix plugin pane commands now default `PWD` to their resolved working directory, so direct popup tools open at explicit `--cwd` paths while preserving caller-provided `PWD` values. (#2984) | ||
| - Plugin link handlers now receive matching OSC 8 `file://` clicks while unmatched file links remain excluded from the system URL opener. (#2941) | ||
| - `pane report-agent` and `pane report-agent-session` now accept the pane ID after their options, as shown by CLI help, and support `--option=value` syntax. (#2926) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both accepted pane-ID positions.
Line 27 says that the pane ID must follow the options. The parser accepts options before or after the pane ID. Update the entry to match the CLI contract and avoid misleading users.
Suggested wording
-- `pane report-agent` and `pane report-agent-session` now accept the pane ID after their options, as shown by CLI help, and support `--option=value` syntax. (`#2926`)
+- `pane report-agent` and `pane report-agent-session` now accept options before or after the pane ID, as shown by CLI help, and support `--option=value` syntax. (`#2926`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `pane report-agent` and `pane report-agent-session` now accept the pane ID after their options, as shown by CLI help, and support `--option=value` syntax. (#2926) | |
| - `pane report-agent` and `pane report-agent-session` now accept options before or after the pane ID, as shown by CLI help, and support `--option=value` syntax. (#2926) |
Greptile SummaryThe PR makes both agent-report commands accept the pane ID before or after their options and supports recognized
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/cli/pane.rs | Updates both agent-report parsers to extract one positional pane ID flexibly and expand recognized equals-form options. |
| tests/cli/panes.rs | Adds end-to-end coverage for options-first reports, embedded equals signs, request success, and resulting agent state. |
Reviews (2): Last reviewed commit: "fix(cli): accept reordered agent report ..." | Re-trigger Greptile
03c8baa to
ffad3ff
Compare
refs herdrdev#2926 Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>
Issue
An integration that follows
pane report-agentorpane report-agent-sessionhelp and places the pane ID after the options gets rejected, so the report never reaches the pane. The equivalent pane-first command works.Problem
CLI help and completions describe normal order-independent options, but these two commands separately assumed the first argument was always the pane ID. An options-first call therefore treated
--sourceas the pane and rejected its value as an unknown option; the same parsers also did not understand--option=value.How did we fix it?
Both agent-report commands now extend the existing flexible pane-argument mechanism: they accept one pane ID anywhere among their options and normalize valued
--option=valuearguments before sending the existing request. This is limited to command-line parsing; pane state ownership, report authority, server behavior, and the existing empty success output are unchanged.Verification
Before the change, both help-ordered commands exited 2 with
unknown option: test:akbashand left the disposable session's agent list empty. After the change, the same commands exited 0, and the state report changed the target pane toworking; an equals-form report with an embedded=value also exited 0 and changed it toblocked.just test-one pane_agent_reports_accept_options_before_panepasses. Formatting, native and Windows Clippy, 98 maintenance tests, integration assets, marketplace tests, and 3,470 other Rust tests pass. The unrelated pre-existinglive_server_holds_one_pty_master_fd_per_panetest could not find its replacement process in this VPS environment and failed again in isolation.refs #2926